Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
WalkthroughThread configurable access-log level and optional stacktrace through logging constructors and config/schema; add per-request log level handling and SubgraphAccessLogger.Error; change engine OnFinished to log errors at ERROR; and expand tests for logging, stacktraces, and broken-pipe handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/pkg/config/testdata/config_full.json (1)
389-390: Consider showcasing non-defaults in full config to exercise behaviorUse this file to demonstrate filtering and toggle:
- Set Level to "warn" (or higher) to show access-log suppression below threshold.
- Optionally set AddStacktrace to false to cover both branches in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/core/graph_server.go(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router/core/graph_server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
router/pkg/config/testdata/config_defaults.json (1)
191-192: Access-logs defaults verified — schema and struct align; re-evaluate stacktrace default
- Schema (router/pkg/config/config.schema.json) and Go struct (router/pkg/config/config.go: AccessLogsConfig) both set level="info" and add_stacktrace=true; testdata (router/pkg/config/testdata/config_defaults.json) uses exported JSON names (Level/AddStacktrace) — expected.
- Level parsing: router/core/supervisor_instance.go uses level.Set(strings.ToUpper(cfg.AccessLogs.Level)) — zapcore parses levels case‑insensitively and the schema enum restricts allowed values.
- Security/privacy: default add_stacktrace=true can leak sensitive data in access logs — confirm compliance requirements. If not acceptable, change schema and AccessLogsConfig default to false or add explicit redaction/guardrails where stacktraces are emitted.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router-tests/structured_logging_test.go (1)
240-240: Consider using explicit log level constant.The calls use
0for the log level parameter, which maps tozapcore.DebugLevel. While functionally correct, usingzapcore.DebugLevelexplicitly would improve code clarity.Apply this change for better readability:
- logger := logging.NewZapAccessLogger(f, 0, false, false, true) + logger := logging.NewZapAccessLogger(f, zapcore.DebugLevel, false, false, true)Also applies to: 343-343
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router-tests/structured_logging_test.go(7 hunks)router/internal/requestlogger/requestlogger.go(5 hunks)router/pkg/config/config.go(1 hunks)router/pkg/config/config.schema.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/config/config.schema.json
- router/pkg/config/config.go
🧰 Additional context used
🧬 Code graph analysis (2)
router/internal/requestlogger/requestlogger.go (1)
router/core/batch.go (1)
Handler(46-58)
router-tests/structured_logging_test.go (4)
router/core/modules.go (3)
EnginePreOriginHandler(122-126)Module(52-54)ModuleInfo(44-50)router/core/context.go (1)
RequestContext(61-137)router/pkg/logging/logging.go (2)
New(21-23)NewZapAccessLogger(109-127)router/core/router.go (3)
Option(172-172)WithCustomModules(1762-1766)WithHeaderRules(1726-1730)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
router/internal/requestlogger/requestlogger.go (5)
52-58: LGTM! Clean separation of concerns.The new
panicLoggerfield cleanly separates panic-specific logging (which should always include stacktraces) from regular access logs. ThelogLevelHandlerfunction field provides flexible per-request log level control.
115-119: LGTM! Well-designed option.The new
WithLogLevelHandleroption follows the existing functional options pattern and enables flexible per-request log level configuration.
127-138: LGTM! Correct initialization.The
panicLoggeris properly initialized withzap.AddStacktrace(zapcore.ErrorLevel), ensuring that panic logs always include stacktraces regardless of the global stacktrace configuration. This is the correct behavior for panic recovery.
175-175: LGTM! Correct panic handling.The panic logging correctly uses
panicLoggerfor regular panics (which includes stacktraces) and suppresses stacktraces for broken pipe errors by temporarily raising the stacktrace level toPanicLevel.Also applies to: 177-177
201-206: LGTM! Dynamic log level implementation.The dynamic log level handling is implemented correctly, defaulting to
InfoLevelwhen no handler is configured and allowing per-request customization via thelogLevelHandlerfunction.router-tests/structured_logging_test.go (6)
7-7: LGTM! Appropriate test dependencies.The new imports (
io,net,syscall,zaptest/observer) provide necessary functionality for simulating broken pipe errors and observing log output in tests.Also applies to: 9-9, 14-14, 23-23
65-90: LGTM! Proper broken pipe simulation.The
MyBrokenPipeModulecorrectly simulates a broken pipe error by creating anet.OpErrorwithsyscall.EPIPE, which matches the error type checked in the panic recovery logic.
3609-3717: LGTM! Comprehensive log level testing.The test suite thoroughly validates the new access log level configuration feature:
- Default behavior (InfoLevel for successful requests)
- Level filtering (logs below configured level are suppressed)
- Error escalation (validation and subgraph errors logged at ErrorLevel)
The test at lines 3633-3659 demonstrates proper use of the observer pattern to verify that logs below the configured level are filtered out.
3719-3845: LGTM! Thorough panic stacktrace testing.The tests correctly verify panic stacktrace behavior:
- Default: Panics always include stacktraces
- Override protection: Even when stacktraces are explicitly disabled, panics still include them (correct behavior for debugging)
- Broken pipe exception: Broken pipe errors don't include stacktraces to avoid log spam, and properly set the
broken_pipefieldThe assertion message at line 3797 clearly documents the expected behavior.
3847-3951: LGTM! Validates breaking change behavior.These tests thoroughly verify the new stacktrace behavior for error access logs:
- Validation errors: Include stacktraces when enabled (lines 3847-3868), omit when disabled (lines 3870-3898)
- Subgraph errors: Include stacktraces when enabled (lines 3900-3921), omit when disabled (lines 3923-3951)
- Error logging: All errors correctly logged at
ErrorLevelThis validates the documented breaking change: stacktraces are now included for all error access logs (not just panics) when the feature is enabled.
3953-3972: LGTM! Verifies successful requests remain clean.This test confirms that successful requests don't include stacktraces, ensuring the feature only activates for error conditions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router/core/supervisor_instance.go (1)
77-80: Consider improving the error message phrasing.The error message "could not parse log level: %w for access logs" places "for access logs" after the error placeholder, which reads awkwardly. Consider rephrasing to "could not parse access log level: %w" or "could not parse log level for access logs: %w" for better clarity.
Otherwise, the level parsing logic is correct and properly handles error cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router-tests/testenv/testenv.go(2 hunks)router/core/supervisor_instance.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router-tests/testenv/testenv.go
🧰 Additional context used
🧬 Code graph analysis (1)
router/core/supervisor_instance.go (2)
router/pkg/mcpserver/util.go (1)
Logger(6-9)router/pkg/logging/logging.go (1)
NewZapAccessLogger(109-127)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build-router
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
router/core/supervisor_instance.go (1)
88-121: LGTM! Level and stacktrace parameters correctly threaded.The implementation correctly threads the parsed log level and stacktrace configuration to all four logger instantiation paths (buffered/non-buffered × file/stdout). The parameter order matches the function signatures, and the logic is consistent across all code paths.
2 participants
This PR:
add_stacktraceflag foraccess_logswhich istrueby default, which when enable would allow any access logs with error to print a stacktrace along with itlevelflag foraccess_logswhich isinfoby default, when set towarnfor example, any access logs abovewarnwonted be printedNote:
We only previously introduced stack traces for panics, we keep that behaviour even if stack traces are disabled.
Summary by CodeRabbit
New Features
Bug Fixes
Tests / Docs
Checklist